!!! TASK: Add workspace content stream mapping to content graph projection #5096

bwaidelich · 2024-05-25T09:38:00Z

Previously the DoctrineDbalContentGraphProjection accessed the workspace table of a different projection in order to resolve the workspace<->content stream mapping.

This change adds the workspace table to the content graph projection and uses that instead for the resolution.

Note: This is not a breaking change because it comes with a migration and does not require a replay, but a

./flow cr:setup

is needed in order to apply that!

Related: #5038
Replaces: #5041

Todos

bwaidelich · 2024-05-25T09:38:31Z

Draft, because it requires some more love

Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php

mhsdesign

by reading ... but not thaaaat deep ... maybe +0.5

Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Projection/Feature/Workspace.php

have to re review after merge of 5097 and further changes

Neos.ContentRepository.Core/Classes/ContentRepositoryReadModel.php

Previously the `DoctrineDbalContentGraphProjection` accessed the `workspace` table of a different projection in order to resolve the workspace<->content stream mapping. This change adds the `workspace` table to the content graph projection and uses that instead for the resolution. **Note:** This is not a breaking change because it comes with a migration and does not require a replay, but a ./flow cr:setup is needed in order to apply that! Related: #5038

wip

In commit 2a781d2 the cache invalidation was partly repaired But as we removed the content graph cache layer currently in 9.0 #5246 A reintroduction will be discussed as part of another change. Related #5039

mhsdesign · 2024-10-15T10:38:07Z

Looks good and works well so far. I also run the Neos Ui e2e and 🔝 🥳
The migration (in cr:setup should probably be tested more extensively .. we especially need to make sure when copying the tables that they have the same schema!!!)

mhsdesign

I went through the whole code again and despite a few nitpicks and maybe on crucial thing left to discuss it looks really promising so far (and as said before works also)
So ill push a followup discuss the last things and then its merge ready id say!

mhsdesign · 2024-10-16T15:47:07Z

Neos.ContentGraph.DoctrineDbalAdapter/src/ContentRepositoryReadModelAdapter.php

+ * @internal only used inside the
+ * @see ContentRepositoryReadModel
+ */
+final readonly class ContentRepositoryReadModelAdapter implements ContentRepositoryReadModelAdapterInterface


💡 fyi (for other reviewers) while these namings are not perfect - they are not the final ones either :)
in #5272 we can discuss the final final final draft:)

mhsdesign · 2024-10-16T15:48:44Z

Neos.ContentGraph.DoctrineDbalAdapter/src/ContentRepositoryReadModelAdapter.php

+                {$this->tableNames->contentStream()}
+            WHERE
+                id = :contentStreamId
+                AND removed = FALSE


As discussed with basti, we are sure that the additional FALSE filter was missing here and should have also been part of the previous findVersionForContentStream or hasContentStream

⚠️ Or is there an edge-case with $commandHandlingDependencies->contentStreamExists where it IS important that the content stream does not exist if pruned or not?
Like if someone prunes a content stream and then voluntarily creates a new workspaces with it? That would crash? Maybe we can add a low level hacky method hasContentStream?

I rethought this again and again and i think the removed state is not just an implementation detail but required to be implemented by the adapter. That means we can and should add it to the ContentStream readmodel so it is clearly documented when this state should be encountered.
Further adding it to the readmodel allows us to remove the findUnusedAndRemovedContentStreamIds in the adapters and interfaces which would be impossible to document due to their complexity. It really feels good to have that now in the ContentStreamPrunner in the core.

see 041f239

After discussing this again we found a way to get rid of the removed flag from the readmodel: by diffing the actual found content streams of the event store and the ones that are in the projection. And as a second step im currently fully overhauling the content stream pruner so that we dont have to track neither the removed nor the content stream state at all in the projection and always extract the information from the events: #5297

So the introduction of ContentStream::removed was just a temporary state and will be removed again :)

mhsdesign · 2024-10-16T15:50:38Z

Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php

-        foreach ($this->determineRequiredSqlStatements() as $statement) {
+        $statements = $this->determineRequiredSqlStatements();
+
+        // MIGRATION from 2024-05-25: copy data from "cr_<crid>_p_workspace"/"cr_<crid>_p_contentstream" to "cr_<crid>_p_graph_workspace"/"cr_<crid>_p_graph_contentstream" tables


❔ do we need to handle the rare cases of the previous workspace/ content stream projection being out of date and behind?
Id say no and for the case this happened one must issue a full replay.

Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php

Neos.ContentRepository.Core/Classes/ContentRepository.php

Neos.ContentRepository.Core/Classes/Service/ContentStreamPruner.php

Neos.ContentRepository.Core/Classes/SharedModel/Workspace/ContentStreams.php

Neos.ContentRepository.Core/Classes/SharedModel/Workspace/Workspace.php

mhsdesign · 2024-10-16T17:13:15Z

Neos.ContentRepository.LegacyNodeMigration/Tests/Behavior/Bootstrap/FeatureContext.php

-
-        $eventNormalizer = $this->getObject(EventNormalizer::class);
+
+        // HACK to access the property converter


💡 fyi ill clean this up in a followup sometime

mhsdesign · 2024-10-16T17:15:49Z

Neos.Neos/Tests/Behavior/Features/Bootstrap/RoutingTrait.php

@@ -185,7 +186,9 @@ public function theMatchedNodeShouldBeInContentStreamAndOriginDimension(string $
        Assert::assertTrue($matchedNodeAddress->workspaceName->isLive());
        Assert::assertSame($nodeAggregateId, $matchedNodeAddress->aggregateId->value);
        // todo useless check?


💡 this can and should be migrated at some point to use workspace names in the routing tests

Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php

mhsdesign · 2024-10-16T17:30:45Z

Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphSchemaBuilder.php

@@ -107,6 +109,30 @@ private function createReferenceRelationTable(): Table
            ->setPrimaryKey(['name', 'position', 'nodeanchorpoint']);
    }

+    private function createWorkspaceTable(): Table


💡 i compared the schemas again against the current ones and can confirm besides a few renames of columns its the same. Thus the migration in setup can definitely work.

Instead of messing with the rather lean API just for this usecase the calculation is done in PHP. This will be slower and consume more memory, but it should not be an issue even with thousands of content streams for this pure cleanup CLI command. Further this calculation is better placed in the Core package than to be done in each adapter because writing the specification for the interface would already be too complex. Additionally, the docs were updated in the `ContentStream` readmodel and now also reference the migrated core `findUnusedAndRemovedContentStreams` logic which also explains the `removed` flag.

…ents with `EmbedsContentStreamId`

…tentstream-mapping-to-contentgraph

LOL

kitsunet · 2024-10-17T08:58:51Z

Neos.Neos/Classes/Service/LinkingService.php


        $mainRequest = $controllerContext->getRequest()->getMainRequest();
        $uriBuilder = clone $controllerContext->getUriBuilder();
        $uriBuilder->setRequest($mainRequest);
-        $createLiveUri = $workspace && $workspace->isPublicWorkspace() && !$resolvedNode->tags->contain(SubtreeTag::disabled());
+        $createLiveUri = $workspace && $nodeAddress->workspaceName->isLive() && !$resolvedNode->tags->contain(SubtreeTag::disabled());


This condition seems a bit disjointed now and smells like a conceptual problem. Aka why do I ask the workspaceName if we are live (I know why, but is that sensible?). Anyways nothing we can solve right now, just wanted to point out that it looks a bit strange and the condition read clearer before.

mhsdesign

With the adjustments and the planned followup #5272 i can definitely approve this!!! Thank you for your initial effort in May where we discussed this - its really fun to see such old pr get merged and its ideas still being valid!!!

Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphSchemaBuilder.php

Just for cosmetics: #5096 (comment)

…ist` The step was previously introduced via neos/neos-development-collection@433f369 but wrongly implemented. see also neos/neos-development-collection#5096 (comment)

Just for cosmetics: neos/neos-development-collection#5096 (comment)

…ist` The step was previously introduced via neos/neos-development-collection@ee0bd14 but wrongly implemented. see also neos/neos-development-collection#5096 (comment)

github-actions bot added Task task/cleanup-doctrinedbaladapter-2 labels May 25, 2024

This was referenced May 25, 2024

WIP: TASK: Add workspace content stream mapping to content graph projection #5086

Closed

Workspace aware ContentGraph / Node followup #5038

Closed

WIP: !!! TASK: Add workspace content stream mapping to content graph projection #5041

Closed

mhsdesign mentioned this pull request May 25, 2024

TASK: Remove --reset-projections flag from cr:setup command #5089

Merged

This was referenced May 25, 2024

Task/replace workspace and contentstreamfinder #5097

Merged

TASK: Cosmetic cleanup of DoctrineDbalContentGraphProjection #5092

Merged

Base automatically changed from task/cleanup-doctrinedbaladapter-2 to 9.0 May 26, 2024 21:15

github-actions bot added 9.0 and removed task/cleanup-doctrinedbaladapter-2 labels May 26, 2024

kitsunet marked this pull request as ready for review May 26, 2024 21:16

mhsdesign reviewed May 27, 2024

View reviewed changes

Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php Outdated Show resolved Hide resolved

mhsdesign previously approved these changes May 27, 2024

View reviewed changes

mhsdesign reviewed May 27, 2024

View reviewed changes

Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Projection/Feature/Workspace.php Outdated Show resolved Hide resolved

bwaidelich changed the title ~~TASK: Add workspace content stream mapping to content graph projection~~ !!! TASK: Add workspace content stream mapping to content graph projection Jun 3, 2024

bwaidelich marked this pull request as draft June 3, 2024 12:41

mhsdesign reviewed Jun 19, 2024

View reviewed changes

Neos.ContentRepository.Core/Classes/ContentRepositoryReadModel.php Show resolved Hide resolved

bwaidelich self-assigned this Sep 18, 2024

bwaidelich mentioned this pull request Oct 1, 2024

WIP: FEATURE: Integrate ContentStream projection into ContentGraph #4980

Closed

1 task

bwaidelich assigned mhsdesign Oct 1, 2024

bwaidelich mentioned this pull request Oct 1, 2024

!!! FEATURE: AssetUsage as CatchUpHook #5258

Merged

mhsdesign mentioned this pull request Oct 1, 2024

!!! FEATURE: Extract workspace metadata and user-assignment to Neos #5146

Merged

bwaidelich and others added 5 commits October 1, 2024 18:16

!!! TASK: Replace dedicated Workspace and ContentStream projections

f3f9571

wip

wip

fab9d2d

wip

a3e6414

TASK: Post rebase phpstan adjustments

17e39a8

bwaidelich and others added 2 commits October 15, 2024 12:02

Fix parameter name in content graph projection

b41f5b5

Revert "TASK: Reintroduce content graph / cr read model cache"

2a8b490

In commit 2a781d2 the cache invalidation was partly repaired But as we removed the content graph cache layer currently in 9.0 #5246 A reintroduction will be discussed as part of another change. Related #5039

mhsdesign requested review from skurfuerst, kitsunet, nezaniel, dlubitz and mhsdesign October 16, 2024 08:07

mhsdesign reviewed Oct 16, 2024

View reviewed changes

Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php Outdated Show resolved Hide resolved

mhsdesign reviewed Oct 16, 2024

View reviewed changes

mhsdesign added 6 commits October 16, 2024 22:39

TASK: Use Workspace->isRootWorkspace()

f604674

TASK: Prevent default arm to throw as we can handle possibly all ev…

acd3833

…ents with `EmbedsContentStreamId`

TASK: Move getContentGraph again upwards

c91f24f

Merge remote-tracking branch 'origin/9.0' into task/add-workspace-con…

738b68b

…tentstream-mapping-to-contentgraph

BUGFIX: Fix logic of Workspace::isRootWorkspace

4a652d2

LOL

kitsunet reviewed Oct 17, 2024

View reviewed changes

mhsdesign approved these changes Oct 17, 2024

View reviewed changes

kitsunet reviewed Oct 17, 2024

View reviewed changes

Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphSchemaBuilder.php Show resolved Hide resolved

kitsunet reviewed Oct 17, 2024

View reviewed changes

Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphSchemaBuilder.php Show resolved Hide resolved

TASK: Adjust casings of columns

a18801b

Just for cosmetics: #5096 (comment)

mhsdesign mentioned this pull request Oct 17, 2024

BUGFIX: Add primary keys and unique indexes on graph projection tables #5296

Merged

6 tasks

kitsunet approved these changes Oct 17, 2024

View reviewed changes

kitsunet merged commit b4fd77b into 9.0 Oct 17, 2024
10 checks passed

mhsdesign deleted the task/add-workspace-contentstream-mapping-to-contentgraph branch October 17, 2024 11:11

mhsdesign mentioned this pull request Oct 17, 2024

FEATURE: Overhaul content stream pruner to work on event stream #5297

Merged

13 tasks

neos-bot pushed a commit to neos/contentgraph-doctrinedbaladapter that referenced this pull request Oct 18, 2024

TASK: Adjust casings of columns

0275b01

Just for cosmetics: neos/neos-development-collection#5096 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

!!! TASK: Add workspace content stream mapping to content graph projection #5096

!!! TASK: Add workspace content stream mapping to content graph projection #5096

bwaidelich commented May 25, 2024 •

edited

Loading

bwaidelich commented May 25, 2024 •

edited

Loading

mhsdesign left a comment •

edited

Loading

mhsdesign commented Oct 15, 2024

mhsdesign left a comment

mhsdesign Oct 16, 2024

mhsdesign Oct 16, 2024

mhsdesign Oct 16, 2024

mhsdesign Oct 16, 2024

mhsdesign Oct 27, 2024 •

edited

Loading

mhsdesign Oct 16, 2024

mhsdesign Oct 16, 2024

mhsdesign Oct 16, 2024

mhsdesign Oct 16, 2024

kitsunet Oct 17, 2024

mhsdesign left a comment


		$eventNormalizer = $this->getObject(EventNormalizer::class);

		// HACK to access the property converter

!!! TASK: Add workspace content stream mapping to content graph projection #5096

!!! TASK: Add workspace content stream mapping to content graph projection #5096

Conversation

bwaidelich commented May 25, 2024 • edited Loading

bwaidelich commented May 25, 2024 • edited Loading

mhsdesign left a comment • edited Loading

Choose a reason for hiding this comment

mhsdesign commented Oct 15, 2024

mhsdesign left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsdesign Oct 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsdesign left a comment

Choose a reason for hiding this comment

bwaidelich commented May 25, 2024 •

edited

Loading

bwaidelich commented May 25, 2024 •

edited

Loading

mhsdesign left a comment •

edited

Loading

mhsdesign Oct 27, 2024 •

edited

Loading